-
Notifications
You must be signed in to change notification settings - Fork 380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(react-grid): correct calculating start index of loading row for Infinite Scrolling #2256
fix(react-grid): correct calculating start index of loading row for Infinite Scrolling #2256
Conversation
FPR |
packages/dx-grid-core/src/plugins/virtual-table-state/helpers.ts
Outdated
Show resolved
Hide resolved
FPR |
FPR |
@@ -143,7 +143,7 @@ export const needFetchMorePages: PureComputed<[VirtualRows, number, number], boo | |||
const { start, end } = intervalUtil.getRowsInterval(virtualRows); | |||
const loadCount = end - start; | |||
const topTriggerIndex = start > 0 ? start + pageSize! : 0; | |||
const bottomTriggerIndex = end - pageSize!; | |||
const bottomTriggerIndex = Math.max(topTriggerIndex + pageSize, end - pageSize! * 1.5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a 1.5
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half of page.
Sometimes difference between bottomTriggerIndex
and topTriggerIndex
more than one pageSize
and we can get index, than no trigger update. This is index in interval between end - pageSize!
and end - pageSize! * 1.5
For example, in master try after load press End
button twice (lazy loading => infinite scrolling demo)
You will stuck at string 150 as last with bottomTriggerIndex === 150
and index ~ 140
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes we may catch this bug not in beginning of table. It is quite difficult to catch, but can. This check fixes it.
const referenceIndex = 320; | ||
|
||
expect(calculateRequestedRange(virtualRows, newInterval, pageSize, referenceIndex)) | ||
.toEqual({ start: 300, end: 400 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want rows 300-400 if they're loaded already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we try load next page (rows 400-500) width index less than page 350 (for pageSize === 100
), we catch index bug. It loads nextPage instead curren when we fast scroll up.
We see at string 320, cache know about previous (200-300) and next page (400-500), and we load 400-500. Then, we get rows combination:
[200,
201,
...
300,
400,
401,
...
500,
401,
...
500]
For index less than start - pageSize/2
we need to shift loading interval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it reproduce in non-infinite mode?
const calculatedRange = intervalUtil.difference(newRange, loadedInterval); | ||
if (isAdjacentPage && calculatedRange !== intervalUtil.empty) { | ||
if (calculatedRange.start - referenceIndex > pageSize / 2) { | ||
calculatedRange.start -= pageSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In non-infinite scrolling mode request range shouldn't be affected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I'll fix it.
FPR |
FPR |
FPR |
Fixes #2139